Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add new site preference for indexing out-of-stock products #218

Open
wants to merge 30 commits into
base: develop
Choose a base branch
from

Conversation

htuzel
Copy link
Collaborator

@htuzel htuzel commented Jan 10, 2025

What is Changed

  • Added new site preference for enabling/disabling to index out of stock products (For full index and delta indexes for different record models)
  • Wrote unit tests
  • Made some logics more modular
  • Improved frontend tests and libnss3, libgbm1, xvfb are added for Chromium/Electron in headless environments. Without them, Cypress tests in Electron or Chrome can fail with “MESA: error: … (VK_ERROR_INCOMPATIBLE_DRIVER)” or “glx: failed to create X screen” errors.

How to Test

Here are my test scenarios:

Test Master Product: 11736753M
Test Variant Product: 883360541075M , 883360541105M

⬇️ Variant out of stock now
⬆️ Variant in stock now
⏬ both Variants (so master) are out of stock now

# In_stock in Master Level Model Job Will Run Update Type (Stock Change) Index out-of-stock products Product Stock Status Scenario result Result
1 False Base Product model ProductIndexJob - true True 11736753M in the index, the variant is in the index
2 False Base Product model ProductIndexJob - true False 11736753M in the index, the variant is in index with in_stock status is false
3 False Base Product model ProductIndexJob - false True 11736753M in the index, the variant is not in index
4 False Base Product model ProductIndexJob - false False 11736753M in the index, variant in_stock status is false
5 True Base Product model ProductIndexJob - true True 11736753M in index and its in_stock status is in base level and it is true
6 True Base Product model ProductIndexJob - true False 11736753M in the index and its in_stock status is in base level and false
7 True Base Product model ProductIndexJob - false True 11736753M in index and its in_stock status is in base level and it is true
8 True Base Product model ProductIndexJob - false False 11736753M in not index
9 False Variation Product model ProductIndexJob - true True it is in the index and the stock is true
10 False Variation Product model ProductIndexJob - true False it is in the index and the stock is false
11 False Variation Product model ProductIndexJob - false True it is in the index and the stock is true
12 False Variation Product model ProductIndexJob - false False it is not in the index
13 True Variation Product model ProductIndexJob - true True NOT A CASE
14 True Variation Product model ProductIndexJob - true False NOT A CASE
15 True Variation Product model ProductIndexJob - false True NOT A CASE
16 True Variation Product model ProductIndexJob - false False NOT A CASE
17 False Base Product model DeltaIndexJob ⬇️ true True Now, the variant is in an array with a false value
18 False Base Product model DeltaIndexJob true False Variants array in index with in_stock false value��I think it is so complicated to delete master products in this scenario ⚠️
19 False Base Product model DeltaIndexJob ⬇️ false False Now, the variant is not in the index
20 False Base Product model DeltaIndexJob ⬆️ false True Now, the variant is in the index
21 False Base Product model DeltaIndexJob false False Variants array in index with in_stock false value��I think it is so complicated to delete master products in this scenario ⚠️
22 True Base Product model DeltaIndexJob ⬆️ true True Requires full indexing
23 True Base Product model DeltaIndexJob ⬇️ true False Requires full indexing
24 True Base Product model DeltaIndexJob true False Requires full indexing
25 True Base Product model DeltaIndexJob ⬆️ false True Requires full indexing
26 True Base Product model DeltaIndexJob ⬇️ false False Requires full indexing
27 True Base Product model DeltaIndexJob false False Requires full indexing
28 False Variation Product model DeltaIndexJob ⬆️ true True In_stock true
29 False Variation Product model DeltaIndexJob ⬇️ true False In_stock false
30 False Variation Product model DeltaIndexJob ⬆️ false True Available in index
31 False Variation Product model DeltaIndexJob ⬇️ false False Not available in the index
32 True Variation Product model DeltaIndexJob true True NOT A CASE
33 True Variation Product model DeltaIndexJob true False NOT A CASE
34 True Variation Product model DeltaIndexJob false True NOT A CASE
35 True Variation Product model DeltaIndexJob false False NOT A CASE

Please also consider this PR regarding moving "In_stock" in the master level.
https://github.com/algolia/doc/pull/9652

Doc PR will be created separately after merge

- Added new site preference for enabling/disabling to index out  of stock products
- Wrote unit tests
@htuzel htuzel changed the title Feat: Added new site preference for enabling/disabling to index out of stock products feat: Added new site preference for enabling/disabling to index out of stock products Jan 10, 2025
@htuzel htuzel requested a review from sbellone January 10, 2025 12:59
@htuzel htuzel self-assigned this Jan 10, 2025
@htuzel htuzel changed the title feat: Added new site preference for enabling/disabling to index out of stock products feat: add new site preference for indexing out-of-stock products Jan 10, 2025
Copy link
Collaborator

@sbellone sbellone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are things missing:

Also, do we want to keep the in_stock attribute when out-of-stock products are not indexed? It doesn't seem useful.

Otherwise, the tests are currently not testing anything.

metadata/algolia/meta/system-objecttype-extensions.xml Outdated Show resolved Hide resolved
@@ -49,6 +49,7 @@ function handleSettings() {
algoliaData.setPreference('EnableContentSearch', algoliaEnableContentSearch);
algoliaData.setPreference('EnableRecommend', algoliaEnableRecommend);
algoliaData.setPreference('EnablePricingLazyLoad', algoliaEnablePricingLazyLoad);
algoliaData.setPreference('IndexOutofStock', params.IndexOutofStock.submitted);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
algoliaData.setPreference('IndexOutofStock', params.IndexOutofStock.submitted);
algoliaData.setPreference('IndexOutOfStock', params.IndexOutOfStock.submitted);

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why have you reverted it back? In the commit message you say:

Updated the preference key from 'IndexOutOfStock' to 'IndexOutofStock' in AlgoliaBM.js to ensure consistency and prevent potential issues in preference handling.

What are the potential issues since it's a new preference that we are adding?

Comment on lines 535 to 537
const localizedProduct = new AlgoliaLocalizedProduct({ product, attributeList: ['objectID'] });

expect(localizedProduct.objectID).toBeNull();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

? The test doesn't check anything regarding skipping out-of-stock products.

Here you are generating an AlgoliaProduct from a SFCC product, with the objectID attribute. But objectID doesn't exist in SFCC products, so it will necessarily be null 🤷

Please take the time to run step by step it in debug to understand what is happening.

Comment on lines 558 to 560
const localizedProduct = new AlgoliaLocalizedProduct({ product, attributeList: ['objectID'] });

expect(localizedProduct).not.toBeNull();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same, there is no relation with indexing out-of-stocks products at all.
You are instantiating a new AlgoliaLocalizedProduct, so it's normal for it to not be null

htuzel and others added 9 commits January 16, 2025 16:24
Updated the preference key from 'IndexOutOfStock' to 'IndexOutofStock' in AlgoliaBM.js to ensure consistency and prevent potential issues in preference handling.
- Introduced a new function `isIncludeOutOfStock` to determine if out-of-stock products should be included based on the `InStockThreshold` and `IndexOutofStock` preferences.
- Updated existing product filtering logic to utilize the new function, ensuring that products are only indexed if they meet the stock criteria.
- Adjusted the `algoliaLocalizedProduct` model to reflect the new out-of-stock handling.
- Modified indexing steps to handle the inclusion and exclusion of products based on their stock status.

This change improves the accuracy of product indexing in Algolia, allowing for better control over which products are indexed based on their availability.
- Removed unnecessary imports and constants related to Algolia data preferences in `jobHelper.js`.
- Simplified the calculation of index names in `algoliaProductDeltaIndex.js` by directly using the locale in the push operation.
- Enhanced unit tests for `jobHelper` and `algoliaLocalizedProduct` to mock Algolia data preferences more effectively, ensuring better test isolation and coverage.
- Updated snapshot tests to reflect changes in the indexing process.
- Updated the `isInclude` function in `productFilter.js` to enhance product filtering by adding comments for clarity and ensuring that option products are excluded from indexing.
- Refactored the `process` function in `algoliaProductIndex.js` to streamline the return logic, improving readability.
- Modified the test in `algoliaLocalizedProduct.test.js` to return `null` instead of an empty array, aligning with expected behavior for better test accuracy.

These changes enhance the accuracy and maintainability of the Algolia product indexing process.
- Introduced a new mock variable `mockRecordModel` to manage the record model state in tests, replacing the previous global preference setting.
- Updated test cases to utilize the new mock variable for setting the record model, ensuring consistency and clarity in test behavior.
- This change enhances the maintainability of the test suite by isolating the record model configuration from global state, improving test reliability.
- Refactored tests in `algoliaLocalizedProduct.test.js` to improve clarity and maintainability.
- Updated the `isIncludeOutOfStock` tests to use more descriptive test names and streamlined mock implementations.
- Ensured that the tests accurately reflect the logic for including or excluding out-of-stock products based on the `IndexOutofStock` preference.
- This change enhances the reliability of the test suite and ensures better coverage of product filtering scenarios.
@htuzel htuzel requested a review from sbellone January 21, 2025 09:49
Copy link
Collaborator

@sbellone sbellone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When some tests fail, you need to understand the root cause before updating them.
To me changing the snapshot is not the correct fix here.

@@ -49,6 +49,7 @@ function handleSettings() {
algoliaData.setPreference('EnableContentSearch', algoliaEnableContentSearch);
algoliaData.setPreference('EnableRecommend', algoliaEnableRecommend);
algoliaData.setPreference('EnablePricingLazyLoad', algoliaEnablePricingLazyLoad);
algoliaData.setPreference('IndexOutofStock', params.IndexOutofStock.submitted);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why have you reverted it back? In the commit message you say:

Updated the preference key from 'IndexOutOfStock' to 'IndexOutofStock' in AlgoliaBM.js to ensure consistency and prevent potential issues in preference handling.

What are the potential issues since it's a new preference that we are adding?

@@ -239,427 +239,4 @@ exports[`process default 1`] = `
]
`;

exports[`process master-level indexing 1`] = `
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Such a big diff in tests result should draw your attention.
Basically the function was previously generating records and now return an empty array.
You have to understand why before updating the test to make it pass. Here, you are potentially hiding a bug!

The problem can just come from the mock, in which case you can update it, but from what I see, your implementation of isIncludeOutOfStock differs from what you had initially done in algoliaLocalizedProduct.js. Is it normal?

- Corrected the spelling of 'IndexOutofStock' in multiple files to ensure consistency in preference handling.
- Integrated the new `productFilter` module in `algoliaLocalizedProduct.js` to handle variant filtering
- Replaced direct inventory and stock threshold checks with a centralized `isIncludeOutOfStock` method
- Simplified variant selection logic by leveraging the new product filter functionality
…exOutOfStock preference

- Added new test case for master-level indexing with IndexOutOfStock=true and product in stock
- Updated existing test case for master-level indexing with IndexOutOfStock=true and product out of stock
- Expanded test coverage to verify different scenarios of product indexing based on stock availability
- Extended the page load timeout in Cypress configuration from the default to 15 seconds
- Helps prevent potential test failures due to slow page rendering or network issues
- Increased page load timeout from 15 to 30 seconds for more stable testing
- Modified test:frontend script to run Cypress with Chrome browser in headed mode
- Removed redundant cypress:run script
@htuzel htuzel requested a review from sbellone January 27, 2025 12:41
- Added system dependencies for Chrome browser testing
- Switched from Electron to Chrome browser in GitHub Actions
- Configured Cypress to run in headed mode for more detailed test output
- Improved code formatting and removed redundant syntax
- Removed explicit page load timeout setting in Cypress configuration
Comment on lines 19 to 20
* If IndexOutOfStock is false, we only return `true` here if the product has
* ATS >= InStockThreshold. If IndexOutOfStock is true, then always return `true`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's very complicated. Again, you must try to come with functions scoped and easy to understand and maintain.

Also, it's always better to write pure functions, i.e. that doesn't change behaviour due to external factor (here, the preferences)

To me the good thing to do here is:

  • Have a simple isInStock(product, inStockThreshold) function that tells you if the product is in stock
  • Do this check of in the jobs:
    if (!productFilter.isInStock(variant, ALGOLIA_IN_STOCK_THRESHOLD) && !INDEX_OUT_OF_STOCK) {
      continue;
    }

.github/workflows/ci.yml Show resolved Hide resolved
var indexName = algoliaData.calculateIndexName('products', locale);
algoliaOperations.push(new jobHelper.AlgoliaOperation(deleteIndexingOperation, { objectID: cpObj.productID }, indexName));
let localeIndexName = algoliaData.calculateIndexName('products', siteLocales[l]);
algoliaOperations.push(new jobHelper.AlgoliaOperation(deleteIndexingOperation, { objectID: cpObj.productID }, localeIndexName));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks ok, but this part of the code is only run when the job is in variant-level mode AND there are no attributes computed from the base product.

When there are attributes computed from the base product such as colorVariations, we don't pass here, we enter in the if (paramRecordModel === MASTER_LEVEL || attributesComputedFromBaseProduct.length > 0) { condition above.
So the logic needs to be updated above too.

- Refactored `productFilter.js` to introduce a more robust `isInStock` method
- Updated multiple indexing scripts to use the new stock filtering approach
- Simplified stock threshold and out-of-stock indexing logic
- Enhanced test coverage for variant-level indexing scenarios
- Removed deprecated `isIncludeOutOfStock` method
- Updated `jobHelper.js` to remove stock filtering from variant record generation
- Modified `algoliaProductDeltaIndex.js` to handle out-of-stock product indexing
- Updated `algoliaProductIndex.js` to skip out-of-stock product records based on configuration
- Improved indexing logic to respect INDEX_OUT_OF_STOCK setting for both master and variant products
…ation

- Cleaned up unnecessary imports in `jobHelper.js`
- Removed references to stock threshold and out-of-stock indexing preferences
- Simplified variant record generation function by eliminating unused dependencies
- Reintroduced page load timeout configuration in Cypress config
- Set timeout to 15 seconds to improve test stability for sandbox environment
@htuzel htuzel requested a review from sbellone January 29, 2025 08:35
Copy link
Collaborator

@sbellone sbellone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more time, please explain what scenarios you've tested.
I ran the most simple scenario possible:

  • Update a product
  • Run the delta job

And I encountered a critical issue.

This is not acceptable.

});
processedVariantsToSend = localizedMaster.variants ? localizedMaster.variants.length : 0;
algoliaOperations.push(new jobHelper.AlgoliaOperation(baseIndexingOperation, localizedMaster, indexName));
let inStock = productFilter.isInStock(product, ALGOLIA_IN_STOCK_THRESHOLD);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can't work! As I explained in #218 (comment) your filtering function differs from the logic in algoliaLocalizedProduct.js
You had improved the logic yourself in #217 to take into account master products!

For master products, your isInStock function ALWAYS returns false
The consequence is that you ALWAYS end up in the else condition where indexName is not defined...

So here is what you would have seen if you had just run the delta job one time:

ERROR CustomJobThread|1976382993|AlgoliaProductDeltaIndex_v2|algoliaProductDeltaIndex
{"message":"No indexName found for this batch request near line:1 column:70","status":400}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really sorry for the back-and-forths and taking your time unnecessarily with these type errors. I will be more careful before requesting your review, and for each PR, I will create detailed test scenarios. Hopefully, it is working now. I also added my test scenarios; there is only two scenarios (similar) that we can think about, but I believe we can ignore them.

Please let me know what you think.

- Updated `productFilter.js` to handle stock availability for master and variant products
- Simplified stock availability checks in `algoliaLocalizedProduct.js`
- Refactored product delta indexing to optimize out-of-stock product handling
- Ensured consistent stock availability filtering across indexing processes
…xing

- Removed unnecessary availability model retrieval in `algoliaLocalizedProduct.js`
- Added timeout for modal backdrop check in Algolia search test
- Ensures modal is not present before proceeding with test steps
@htuzel htuzel requested review from sbellone and removed request for sbellone January 30, 2025 23:46
- Updated `productFilter.js` to improve stock availability checks
- Modified `jobHelper.js` to filter out variants based on product inclusion criteria
- Adjusted `algoliaProductDeltaIndex.js` to handle index name calculation for localized products
- Simplified stock availability logic across product indexing processes
@htuzel htuzel requested a review from sbellone January 31, 2025 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants